Modifications to run on openmind, refactoring, and auto-formatting. #10
Conversation
|
Something I just noticed @nwatters01 The new separation of the interfaces into the Did you mean to delete the original |
| reward_line = TimeSeries( | ||
| name='reward_line', | ||
| data=H5DataIO(self._reward_line, compression='gzip'), | ||
| unit='reward line open', |
There was a problem hiding this comment.
The unit field is generally something expected to be in SI units (no check yet on the Inspector due to difficulty agreeing on an exact substandard of text implementation of complex unit combinations NeurodataWithoutBorders/nwbinspector#208)
What does this new data stream look like? I don't recall seeing it in the initial round of example data
There was a problem hiding this comment.
It's a binary array. 1 means reward line is open, 0 means reward line is closed. You're right it was not in the initial example data, but I think for open-source data it's important to include exactly when the reward line was open and closed (sometimes monkey was given reward for reasons other than it's response, e.g. for fixating), hence why I added this.
What do you suggest we do with this data stream?
There was a problem hiding this comment.
Great, thanks for explaining. That sounds like some great data to include!
I'd strongly recommend using a LabeledEvents structure from ndx-events instead of a generic TimeSeries for this type of data
Let me know if any questions about how to adapt to that alternate structure
|
@nwatters01 The summary of changes seemed to miss an important aspect: you also added two new data interfaces for data streams we haven't seen before So I'm giving that side of this PR the usual more rigorous look over for best practices, etc. BTW how do you feel about |
Co-authored-by: Cody Baker <[email protected]>
Co-authored-by: Cody Baker <[email protected]>
|
@CodyCBakerPhD Thank you for the review! I really appreciate all comments and suggestions --- since this serves as a template conversion library for the lab, I absolutely want it to adhere to all of your best practices! Yes I meant to remove behavior_interface.py. Just did that and am updating the PR. And yes I added the reward and audio data streams. I've never used isort before, but if you prefer using that, I'm happy to give it a try --- what does it offer? |
Orders global imports by (a) those belonging to Python standard, (b) external packages, (c) locally defined modules We have always followed this convention ourselves ( |
|
Oh, that sounds good. I'll apply isort and update the PR. |
|
@nwatters01 One small conflict here now - also I didn't realize the pre-commit bot wasn't even running yet on this repo so I enabled that |
|
I ran into a github rabbit hole trying to merge this repo with my fork, so instead I made a branch "nwatters" to this repo with all of these changes. I think at this point the only thing I need to do is change to LabeledEvents structure, then that branch will be ready for merging. |
|
In light of the github troubles I'm having with my fork, can we delete this pull request and switch to this one? That has all the same changes, but is actually merge-able into the main repo. |
|
replaced by #13 |
Summary of changes: